Skip to content

Conversation

@jessevz
Copy link
Contributor

@jessevz jessevz commented Nov 12, 2025

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new helper endpoint /api/v2/helper/getTaskProgressImage that generates PNG images visualizing the progress of tasks and supertasks. The implementation is based on the existing task image generation code from api/taskimg.php but adapted for the APIv2 architecture.

Key Changes

  • New GET endpoint that accepts either a task or supertask query parameter
  • Generates a 1500x32 pixel PNG image showing progress through colored rectangles (grey=pending, yellow=in-progress, blue=complete, green=cracked)
  • Implements the AbstractHelperAPI pattern with PSR-7 request/response handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return $response->withStatus(200)
->withHeader("Content-Type", "image/png")
->withHeader("Cache-Control", "no-cache");
}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing resource cleanup. The image resource created by imagecreatetruecolor() on line 108 is never explicitly destroyed with imagedestroy($image). While PHP will clean this up eventually, it's best practice to explicitly free image resources after use, especially for endpoints that may be called frequently.

Copilot uses AI. Check for mistakes.
Comment on lines 44 to 67
public function getParamsSwagger(): array {
return [
[
"in" => "query",
"name" => "supertask",
"schema" => [
"type" => "integer",
"format" => "int32"
],
"required" => true,
"example" => 1,
"description" => "The ID of the supertask where you want to create the progress image of."
],
[
"in" => "query",
"name" => "task",
"schema" => [
"type" => "integer",
"format" => "int32"
],
"required" => true,
"example" => 1,
"description" => "The ID of the task where you want to create the progress image of."
]
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicting parameter requirements in Swagger documentation. Both supertask and task parameters are marked as "required" => true (lines 53 and 64), but the actual logic shows they are mutually exclusive - only one is needed, not both. The requirement should be that at least one must be provided. Consider marking both as not required individually and documenting in the descriptions that one of the two must be provided.

Copilot uses AI. Check for mistakes.


#[NoReturn] public function actionPost(array $data): object|array|null {
assert(False, "GetTaskProgressImage has no POST");
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python-style boolean False used in PHP assertion. PHP uses lowercase false. This will cause a fatal error if the code path is ever reached and assertions are enabled, as False is an undefined constant in PHP (unless defined elsewhere).

Suggested change
assert(False, "GetTaskProgressImage has no POST");
assert(false, "GetTaskProgressImage has no POST");

Copilot uses AI. Check for mistakes.
//prepare image
imagefill($image, 0, 0, $transparency);

if ($taskWrapper->getTaskType() == DTaskTypes::SUPERTASK && isset($supertask_id)) {
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent variable usage in conditional check. Line 122 checks isset($supertask_id) but line 96 uses else if ($supertask_id) which will be truthy even if $supertask_id is 0 or empty string. The logic should be consistent - either use isset() in both places or check for truthiness in both. Additionally, the variable may not be set if neither parameter is provided (which is now only caught by the final else on line 101).

Copilot uses AI. Check for mistakes.
if ($task_id) {
$task = Factory::getTaskFactory()->get($task_id);
if ($task == null) {
throw new HttpNotFoundException($request, "Invalid file");
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message doesn't match the context. The error message says "Invalid file" but this endpoint is about tasks, not files. Should be something like "Invalid task" or "Task not found".

Suggested change
throw new HttpNotFoundException($request, "Invalid file");
throw new HttpNotFoundException($request, "Invalid task");

Copilot uses AI. Check for mistakes.
$keyspace = max($task->getKeyspace(), 1);

//load chunks
$qF = new QueryFilter(Task::TASK_ID, $task->getId(), "=");
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong QueryFilter field. This should use Chunk::TASK_ID instead of Task::TASK_ID since we're filtering chunks by task ID, not tasks. The Chunk model has a TASK_ID field that should be used here.

Suggested change
$qF = new QueryFilter(Task::TASK_ID, $task->getId(), "=");
$qF = new QueryFilter(Chunk::TASK_ID, $task->getId(), "=");

Copilot uses AI. Check for mistakes.
Comment on lines 203 to 204
//send image data to output
imagepng($image);
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing image output to response body. The function calls imagepng($image) on line 204 which outputs directly to stdout, but doesn't capture this output to the response body. This will likely cause issues with the PSR-7 response handling. Consider using imagepng($image, null) and capturing the output with ob_start()/ob_get_clean(), then writing it to $response->getBody()->write().

Suggested change
//send image data to output
imagepng($image);
//send image data to output
ob_start();
imagepng($image);
$imageData = ob_get_clean();
imagedestroy($image);
$response->getBody()->write($imageData);

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +83
$task_id = $request->getQueryParams()['task'];
$supertask_id = $request->getQueryParams()['supertask'];
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation for required query parameters. Unlike similar endpoints (e.g., getFile.routes.php), this code doesn't check if the required task or supertask parameters exist before accessing them. This will cause PHP notices/warnings if neither parameter is provided. Add validation: if (!isset($request->getQueryParams()['task']) && !isset($request->getQueryParams()['supertask'])) before lines 82-83.

Suggested change
$task_id = $request->getQueryParams()['task'];
$supertask_id = $request->getQueryParams()['supertask'];
$queryParams = $request->getQueryParams();
if (!isset($queryParams['task']) && !isset($queryParams['supertask'])) {
throw new HttpError("No task or super task has been provided");
}
$task_id = isset($queryParams['task']) ? $queryParams['task'] : null;
$supertask_id = isset($queryParams['supertask']) ? $queryParams['supertask'] : null;

Copilot uses AI. Check for mistakes.
}
}
}
else {
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential undefined variable usage. If the $supertask_id path is taken (lines 96-100), the variable $task is never defined, but it's used in line 156 within the else block. This will cause an error when accessing a supertask but falling into the non-supertask rendering path. The conditional logic on line 122 should prevent this, but the code structure is fragile and could lead to bugs.

Suggested change
else {
else {
if (!isset($task)) {
throw new HttpError("Task is not defined for non-supertask rendering path.");
}

Copilot uses AI. Check for mistakes.
Comment on lines +135 to +136
$qF = new QueryFilter(Chunk::TASK_ID, $tasks[$i]->getId(), "=");
$chunks = Factory::getChunkFactory()->filter([Factory::FILTER => $qF]);
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate code: Lines 129-131 and 135-137 perform identical database queries to fetch chunks. The query on lines 135-137 is redundant and should be removed. The $chunks variable from lines 129-131 should be reused.

Suggested change
$qF = new QueryFilter(Chunk::TASK_ID, $tasks[$i]->getId(), "=");
$chunks = Factory::getChunkFactory()->filter([Factory::FILTER => $qF]);

Copilot uses AI. Check for mistakes.
@jessevz jessevz marked this pull request as draft November 13, 2025 07:57
@jessevz jessevz marked this pull request as ready for review November 13, 2025 11:22
@jessevz jessevz merged commit a441712 into dev Nov 13, 2025
2 checks passed
@jessevz jessevz deleted the 1747-bug-helper-for-generating-an-image-for-the-visual-graph branch November 13, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants